-
Notifications
You must be signed in to change notification settings - Fork 268
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add confirmation dialog when deleting objects #5445
Conversation
TODO:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been thinking about this for a while, and agree we need to do it. +1 to approach with ListItemButtonIcon where we can just add a confirmation attribute.
I think we've already heard from a couple users that don't always want this on for every object. I'd still say we do this first and be ready to have a discussion about a 'do not show again' and whether preference should be per object, per environment, etc. This approach would work, could always do something like export let confirm: boolean | string
(preference key).
I don't love how the tests change, especially ones less related to the actions like *Details and *ListCompose. Instead of having to hook all message boxes to respond with 0 it feels like the tests should just say 'Say yes to confirmation dialog(s)'. For now maybe it would be better to at least put the message box mocking in a test helper util?
582661e
to
9547bf8
Compare
I agree! Do we happen to already have a utility folder for svelte front-end-related tests? I wasn't able to find a helper file / directory that we could use. If not, I can throw it in a new folder. @cbr7 @odockal I'm unsure what I'm doing wrong, but I'm trying to create a helper function to click the confirmation dialog for the e2e tests, if you're able to point out a fix that'd be awesome |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried this on volumes and services, it is working great. 👍🏼 Just one comment inline.
Nit: it says 'Do you want to delete volume", feels like it should say "Do you want to delete the volume" but this is not required.
We could update that in a separate PR as we'd have to update the titles in most places: https://github.com/containers/podman-desktop/pull/5445/files#diff-1fb3450fd535b8bfd2e357f46b03ba71a318831bcb449a065d509b27ebd53c05R16 |
f97d702
to
26ca2c5
Compare
@cdrage I am afraid that you have overwritten commit that fixed e2e tests, so it fails again. |
Ah sorry, do you happen to have your commit still available? For some reason with the rebase it did overwrite. |
26ca2c5
to
27c729f
Compare
27c729f
to
aee13f1
Compare
@cdrage Seems that we are all set for the merge? |
Yup 💯 just need a re-review! |
I have had a chance to test the PR locally on Linux due to e2e tests updates, so it is +1 from me as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
9f355c6
to
1ab69f0
Compare
### What does this PR do? Adds confirmation dialogs to all critical components of Podman Desktop where you are deleting a container, pod, deployment, service, etc. This makes sure you do not accidently delete something (such as a deployment) that could be critical infrastructure. ### Screenshot / video of UI <!-- If this PR is changing UI, please include screenshots or screencasts showing the difference --> ### What issues does this PR fix or reference? <!-- Include any related issues from Podman Desktop repository (or from another issue tracker). --> Closes containers#5444 ### How to test this PR? Try to delete anything (volume, pod, container, etc.) and you will now see a dialog. <!-- Please explain steps to reproduce --> Signed-off-by: Charlie Drage <charlie@charliedrage.com>
…mation dialog Signed-off-by: Ondrej Dockal <odockal@redhat.com>
Signed-off-by: Charlie Drage <charlie@charliedrage.com>
c0a5af5
to
8d9c533
Compare
Signed-off-by: Charlie Drage <charlie@charliedrage.com>
8d9c533
to
fa6d680
Compare
Ready for a re-review 👍 Got e2e tests working thanks to help from @odockal was having difficulty why aria-label wasn't working.. then realized it was getting it from the wrong dialog. |
feat: add confirmation dialog when deleting objects
What does this PR do?
Adds confirmation dialogs to all critical components of Podman Desktop
where you are deleting a container, pod, deployment, service, etc.
This makes sure you do not accidently delete something (such as a
deployment) that could be critical infrastructure.
Screenshot / video of UI
Screen.Recording.2024-01-05.at.5.05.44.PM.mov
What issues does this PR fix or reference?
Closes #5444
How to test this PR?
Try to delete anything (volume, pod, container, etc.) and you will now
see a dialog.
Signed-off-by: Charlie Drage charlie@charliedrage.com